Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(scanner): add '--packages-depth' parameter. #8372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fb33
Copy link
Contributor

@fb33 fb33 commented Mar 4, 2024

Sometimes we don't want to audit the whole dependencies of a project but only those declared in the project's package manager files.
To do this, I add a new parameter to the scanner to manage the depth in the dependency tree to scan.

@fb33 fb33 requested a review from a team as a code owner March 4, 2024 10:15
The '--packages-depth' parameter allows not to scan the whole
dependency tree. For example, if you use 'scan --packages-depth 1'
then the scan will be performed on projects and the first level of
packages (i.e. the direct dependencies).
By default, the value is -1, which means no limit, all packages
will be scanned.

Signed-off-by: François Barbe <[email protected]>
@fb33 fb33 force-pushed the feat/packages-depth branch from b28abce to a0f8f8c Compare March 4, 2024 10:20
@fviernau
Copy link
Member

fviernau commented Mar 4, 2024

We may consider to add a helper command to strip out dependencies for a given depth from an analyzer result as an alternative for supporting this use case. (That would not add complexity to the scanner)

@sschuberth
Copy link
Member

Thank for the contribution @fb33!

A basic issue I see with the implementation is related to ORT's transparency about what has been scanned: When only looking at the ORT scan result, how do you know whether only a subset of the packages have been scanned vs. really only the listed findings were present?

What I'm trying to say is: We need a way to record as part of the ORT result / in the scan result that only a subset (and which subset) of packages has been scanned. This could probably be solved as easily as adding the used depth (if != -1) to the scan result.

But thinking further, if only scanning of e.g. direct dependencies is desired, should we also limit analysis to the configured depth already? That would automatically limit the scanning to those packages. Related issues: #2293, #5626, #8361

@fb33
Copy link
Contributor Author

fb33 commented Mar 4, 2024

Hi,
My request seems very closed to the issue #5626
I didn't know that ort record a global scan result, I thought it was by package... So, yes the used depth should be added to the scan result in logic of this PR.

by the way, I'll think about your proposal to limit the depth at the analyzer time.
But my use case, I want to have the whole tree to find some security issues but just the direct dependencies for FOSS concern (licenses & copyrights).

@tsteenbe
Copy link
Member

This pull request was discussed in the ORT community meeting of March 21st, 2024.

The people attending can see the usefulness of this feature as:

  • Limiting the result saves work and enable user to gradually improve their compliance over time e.g. first up to level 1 then 3, 6 etc.
  • Don't want to do a full depth scan when open-sourcing code - you only care about first-level
  • Limiting the scan will give user faster results which may be useful within running ORT in pull requests

However there are various concerns:

  • Legally the depth of what is scanned is irrelevant. If OSS is included you have to respect its licenses
  • End-to-end traceability, how can we make it visible in reports that not a full scan was done. We should implement a way to make ORT configurations visible.
  • Maintainability of the pull request
  • Should we instead of CLI parameter implement this within config.yml

Question is if we should protect users against themselves? Given other options in ORT our believe we should not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants